I'd vote for keeping just the single unique_ptr containing all the state, maybe even keeping the original File struct forward-declared above. Easier to clean up on close, less includes in the header.

✔️ done

<cstdlib> .. why is it needed, by the way?

I think that was because of strtol and strtof

✔️ done

Please no using namespace.

🤔 But it makes alot of things more convenient... Aaaand it's a "private" file? Obviously explicitness is often nicer, but is there any other reason for this?

It's not used on that many places ;)

My reasoning: if a namespace is annoyingly long to type, it indicates a problem with the API naming -- it should be shortened. Using using namespace would somewhat hide that problem, but not solve it, and also makes the code a bit more opaque for people that don't know what belongs to the used namespace. I would say that in this case the namespace is still bearable ;)

No need to delete default constructors if there are other constructors defined. Also, would make the other one explicit. Similarly below.

✔️ done

Coding style: please use /* */-style comments, the // ones are treated as temporary to-be-removed comments.

I know! :D It is a temporary comment, so I don't forget to put decent wording for other people there.

Sorry :)

✔️ done (Cleanly formulated all the helper documentations and checked for Captial letter at comment starts everywhere)

Why is this removed? Then there's no test case that tests that mesh3DForName("") returns -1

Well, for the empty file, we now don't load anything at all...
I could put a mesh there, and then check the mesh3DForName(""). But why should it be returning -1 anyway?

Ah, I see. Would be nice to have the check for both unnamed object and mesh then.

-1 is because there can be multiple meshes with empty name -- so you can map their ID to an empty name but not the other way around.

Is there a test case for the actual material data import?

Good point.

If the material would be fully parsed in this function instead of in parse(), you wouldn't need the temporary objMat structure I think.

I hope I'm not talking about impossible things here, though ;)

Sure, I guess it would be enough to only parse the material names in parse() and maybe store their location in ArrayView<const char>s instead

Again, this is done right in the next loop iteration, no need to do this here as well.

Again, no, the ignore Whitespaces ignores words before the keyword (nextWord() expects first character to be non-whitespace, otherwise will return empty string, indicating there is no word at the beginning), versus the other one skips whitespaces after the keyword.

Maybe Wikipedia has it wrong, but I saw some prefix whitespace in the code samples there: https://en.wikipedia.org/wiki/Wavefront_.obj_file#File_format

I personally don't see why there should be such a restriction of no whitespace at line beginning.

I never said it was a restriction, the parser will parse ······\t····p·1 just fine with my current implementation?

Ah! The point is that I ignore those newlines before using nextWord(). Hence the input to that will always be p·1 in the example above, because the others have already been ignored.

the parser will parse ······\t····p·1 just fine with my current implementation?

My point is that this wouldn't work if this would be the very first line (as far as I can see), so I'm suggesting putting the skipWhitespaces() as the very first thing in the loop, instead of very last ;)

Hmm, alright, yeah, I seem to have removed the first skipWhitespace call somewhere, but what you say makes sense

I'm a bit unsure about these inline lambdas -- i'm personally always trying to do such cleanup code only once in the parsing loop, because otherwise the corner-case testing gets quite out of hand. But I didn't stare at the code long enough yet to see how that could be done :)

This will not be easy, will stare at the code for a while and tell you what I think after.

Oh, forgot to say: could the internal Obj* structs be wrapped in an anonymous namespace as well to prevent symbol leakages? Thank you.

✔️ done

Thank you for putting it all in one -- could it be ObjImporter::State or something so it doesn't appear in the global Trade namespace? There is a leftover struct File forward-declaration in the header also, so you could just reuse that line.

Previously the state unique_ptr was reset in doOpen*(), but those two added members in and fileRoot shouldn't be, because they are set before the state is reset. Hence two options:
Either unique_ptr<File> with a new struct File in the Importer state struct (which can be reset independently from the importer state) or revert back to having the two members outside of this struct.

Took me a while to realize what's going on, sorry for the late reply.

but those two added members in and fileRoot shouldn't be, because they are set before the state is reset

I think they don't need to be. One option:

void ObjImporter::doOpenFile(const std::string& filename) {
    AbstractImporter::doOpenFile(filename);
    _state->fileRoot = Utility::Directory::path(filename);
}

void ObjImporter::doOpenData(Containers::ArrayView<const char> data) {
    _state.reset(new ImporterState);
    _state->in = Containers::Array<char>{data.size()};
    std::copy(data.begin(), data.end(), _state->in.begin());
    parse();
}

The above assumes that parse() doesn't need to access the _fileRoot parameter (which it shouldn't, if you defer the material parsing to later, as I suggested). Another option could be this, where the _state is only populated if it's not already there:

void ObjImporter::doOpenFile(const std::string& filename) {
    _state.reset(new ImporterState);
    _state->fileRoot = Utility::Directory::path(filename);
    AbstractImporter::doOpenFile(filename);
}

void ObjImporter::doOpenData(Containers::ArrayView<const char> data) {
    if(!_state) _state.reset(new ImporterState);
    _state->in = Containers::Array<char>{data.size()};
    std::copy(data.begin(), data.end(), _state->in.begin());
    parse();
}

The AbstractImporter::open*() function implementation is unconditionally calling close() before calling the doOpen() implementations, so you can be always sure that if _state is already populated, it's yours and not some stale one from a previous file.